fix: write-buffer-size overflow #3193
Conversation
WalkthroughChanged PikaConf::SetWriteBufferSize to accept a 64-bit value, updated its call site, added an integration test that sets Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/pika_admin.cc (1)
2787-2799: Setter call now correctly avoids 32‑bit truncationPassing
ivaldirectly intoSetWriteBufferSize(int64_t)removes the previous narrowing to 32‑bit and aligns with theint64_tbacking field, which should fix the negative value regression for values > 2,147,483,647.As a low‑priority cleanup, you could consider making
ivalexplicitlyint64_tto make the 64‑bit intent clearer in this function, but behavior is already correct as written.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
include/pika_conf.h(1 hunks)src/pika_admin.cc(1 hunks)tests/integration/server_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/integration/server_test.go (1)
src/pika_admin.cc (4)
ConfigSet(2317-3098)ConfigSet(2317-2317)ConfigGet(1583-2314)ConfigGet(1583-1583)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_on_centos
- GitHub Check: build_on_ubuntu
- GitHub Check: build_on_macos
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
tests/integration/server_test.go (1)
348-358: New large write-buffer-size regression test looks solidThe test cleanly exercises the >INT_MAX case, checks both CONFIG SET success and exact CONFIG GET value, and aligns with the reported regression. No issues from a correctness perspective.
include/pika_conf.h (1)
774-778: write-buffer-size setter correctly widened to int64_tThe setter’s signature and implementation now match the
int64_tbacking field and getter, preserving thread safety while allowing largewrite-buffer-sizevalues to be stored and rewritten without overflow.
There was a problem hiding this comment.
Pull request overview
This PR fixes an integer overflow bug in the write-buffer-size configuration parameter. When values larger than 2147483647 (INT32_MAX) were set, the value would overflow and become negative due to an inappropriate cast from long int (parsed by pstd::string2int) to int in the setter call.
Key Changes
- Changed the
SetWriteBufferSizeparameter type fromconst int&toint64_tto match the underlying member variable type - Removed the problematic
static_cast<int>(ival)cast that caused overflow when passing values to the setter - Added an integration test validating that large write-buffer-size values (3000000000) are correctly set and retrieved
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| include/pika_conf.h | Updated SetWriteBufferSize parameter type from const int& to int64_t, consistent with the member variable type and other 64-bit setters |
| src/pika_admin.cc | Removed the static_cast<int>(ival) cast when calling SetWriteBufferSize, allowing the full int64_t value to be passed |
| tests/integration/server_test.go | Added integration test verifying write-buffer-size values larger than INT32_MAX are handled correctly |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pika.yml(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Analyze (go)
- GitHub Check: build_on_macos
- GitHub Check: build_on_ubuntu
- GitHub Check: build_on_centos
🔇 Additional comments (1)
.github/workflows/pika.yml (1)
216-216: Could you provide the contents of.github/workflows/pika.ymlaround lines 216-240, or additional context about the PR's stated purpose? This will allow me to verify whether the scope creep concern is valid and whether the GCC prefix detection implementation handles edge cases correctly.
| rm -rf /__w/pikiwidb/pikiwidb/buildtrees 2>/dev/null || true | ||
| rm -rf /__w/pikiwidb/pikiwidb/deps 2>/dev/null || true | ||
| find /__w/pikiwidb/pikiwidb -type f \( -name "librocksdb.a" -o -name "libprotoc.a" -o -name "libprotobuf.a" \) -delete 2>/dev/null || true | ||
| find /__w/pikiwidb/pikiwidb -type f \( -name "*.o" -o -name "*.a" -o -name "*.la" -o -name "*.so" -o -name "*_test" \) ! -path "*/build/pika" -delete 2>/dev/null || true | ||
| rm -rf /__w/pikiwidb/pikiwidb/.git 2>/dev/null || true |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the find pattern correctly excludes build/pika artifacts
# Create test directory structure
TEST_DIR=$(mktemp -d)
mkdir -p "$TEST_DIR/build/pika/bin"
mkdir -p "$TEST_DIR/other/test"
touch "$TEST_DIR/build/pika/bin/pika"
touch "$TEST_DIR/build/pika/lib.a"
touch "$TEST_DIR/other/test/lib.a"
echo "=== Files that WILL be deleted (should NOT include build/pika) ==="
find "$TEST_DIR" -type f \( -name "*.o" -o -name "*.a" \) ! -path "*/build/pika" -print
echo ""
echo "=== Files under build/pika (should be preserved) ==="
find "$TEST_DIR/build/pika" -type f
# Additional test: verify with recursive paths
echo ""
echo "=== Testing with nested paths under build/pika ==="
mkdir -p "$TEST_DIR/build/pika/subdir"
touch "$TEST_DIR/build/pika/subdir/lib.a"
echo "With nested path:"
find "$TEST_DIR" -type f \( -name "*.o" -o -name "*.a" \) ! -path "*/build/pika*" -print
echo ""
echo "Original pattern (without asterisk after pika):"
find "$TEST_DIR" -type f \( -name "*.o" -o -name "*.a" \) ! -path "*/build/pika" -print
# Cleanup
rm -rf "$TEST_DIR"Repository: OpenAtomFoundation/pikiwidb
Length of output: 198
🏁 Script executed:
#!/bin/bash
# Test find pattern behavior - NO repo access needed
# Create test directory structure
TEST_DIR=$(mktemp -d)
mkdir -p "$TEST_DIR/build/pika/bin"
mkdir -p "$TEST_DIR/other/test"
touch "$TEST_DIR/build/pika/bin/pika"
touch "$TEST_DIR/build/pika/lib.a"
touch "$TEST_DIR/other/test/lib.a"
touch "$TEST_DIR/test.a"
echo "=== Directory structure ==="
find "$TEST_DIR" -type f
echo ""
echo "=== Testing pattern: ! -path '*/build/pika' ==="
echo "Files WILL be deleted with this pattern:"
find "$TEST_DIR" -type f \( -name "*.a" \) ! -path "*/build/pika" -print
echo ""
echo "=== Testing pattern: ! -path '*/build/pika*' (with asterisk) ==="
echo "Files WILL be deleted with improved pattern:"
find "$TEST_DIR" -type f \( -name "*.a" \) ! -path "*/build/pika*" -print
echo ""
echo "=== Testing pattern: ! -path '*/build/pika/*' (with trailing slash) ==="
echo "Files WILL be deleted with slash pattern:"
find "$TEST_DIR" -type f \( -name "*.a" \) ! -path "*/build/pika/*" -print
# Cleanup
rm -rf "$TEST_DIR"Repository: OpenAtomFoundation/pikiwidb
Length of output: 198
🌐 Web query:
find -path glob pattern matching behavior exclude directory
💡 Result:
Short summary
- find -path matches the whole file path (relative to the start point) using shell globs; '/' and '.' are not special to the pattern. [1]
- To exclude a directory and its contents efficiently, use -prune together with a -path that matches the directory exactly (including the same prefix as the find start point), then use -o for the rest of the expression. The ordering matters: prune must be evaluated before tests like -type. [1][2]
Typical examples
-
Exclude a directory named dir_to_exclude under the current dir:
find . -path "./dir_to_exclude" -prune -o -print
(prints everything except dir_to_exclude and its contents). [2] -
Exclude by pattern (all files under any dir named node_modules):
find . -path "/node_modules/" -prune -o -name "*.js" -print
(prunes those trees, then finds .js elsewhere). [2][3]
Notes and pitfalls
- Make sure your -path pattern matches exactly how find would print the path (e.g., include ./ when starting at . or include the leading directory when starting elsewhere). Otherwise prune may not match. [2][3]
- Using -not -path only prevents printing matches but still descends into excluded trees; use -prune to avoid recursing into them. [2][3]
Sources: man page for find ( -path / -prune ) and common examples/discussion. [1][2][3]
References
[1] find man page (–path description).
[2] Stack Overflow — "How do I exclude a directory when using find?" (examples using -prune).
[3] Stack Overflow — examples and explanation of -path matching and -prune.
Fix find pattern to correctly preserve build/pika artifacts.
The pattern ! -path "*/build/pika" will not exclude files under the build/pika directory. A file like build/pika/lib.a has the full path build/pika/lib.a, which does not match the glob pattern */build/pika (the pattern only matches the directory name itself, not paths within it). Use ! -path "*/build/pika/*" instead. Better yet, use -prune for both correctness and efficiency: find ... -path "*/build/pika" -prune -o \( -name "*.o" -o -name "*.a" ... \) -delete.
🤖 Prompt for AI Agents
.github/workflows/pika.yml lines 173-177: the find command's exclusion `! -path
"*/build/pika"` doesn't exclude files under build/pika (it only matches the
directory itself); update the command to either change the exclusion to `! -path
"*/build/pika/*"` or, preferably, use find's pruning for correctness and
efficiency — i.e. add `-path "*/build/pika" -prune -o` before the name tests so
files under that directory are skipped, then keep the name predicates and
`-delete`.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/storage/tests/hashes_test.cc(1 hunks)
🧰 Additional context used
🪛 Cppcheck (2.18.0)
src/storage/tests/hashes_test.cc
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_on_macos
- GitHub Check: build_on_centos
- GitHub Check: build_on_ubuntu
- GitHub Check: Analyze (go)
src/storage/tests/hashes_test.cc
Outdated
| s = db.HIncrbyfloat("HINCRBYFLOAT_KEY", "HINCRBYFLOAT_FIELD", "12.3456", &new_value); | ||
| ASSERT_TRUE(s.ok()); | ||
| ASSERT_EQ(new_value, "12.3456"); | ||
| ASSERT_NEAR(std::stod(new_value.ToString()), 12.3456, 1e-9); |
There was a problem hiding this comment.
Unrelated change: scope creep in bug-fix PR.
This floating-point comparison improvement is unrelated to the write-buffer-size overflow fix described in the PR objectives. While using ASSERT_NEAR for floating-point comparisons is technically correct and preferred over string equality, this change should be in a separate PR focused on test improvements.
Additionally, line 391 contains a commented-out similar assertion (//ASSERT_EQ(new_value, "12.3456");), suggesting this fix is incomplete. If you're addressing floating-point comparison issues in tests, all instances should be handled consistently.
Recommendation: Consider moving this test improvement to a separate PR and ensure all floating-point comparisons in the test are handled consistently (including the commented-out assertion at line 391).
🧰 Tools
🪛 Cppcheck (2.18.0)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
🤖 Prompt for AI Agents
In src/storage/tests/hashes_test.cc around line 388, the test change replacing a
string equality with ASSERT_NEAR is unrelated to the write-buffer-size overflow
fix and introduces scope creep; revert this change in this PR (restore the
original assertion) and move the floating-point comparison improvement to a
separate test-improvement PR where you should consistently update all similar
checks (including the commented-out ASSERT_EQ at ~line 391) to use an
appropriate floating-point assertion such as ASSERT_NEAR with a defined epsilon.
1 participant
fix #3183
修复在 pika v3.5.5中设置 write-buffer-size 值大于 2147483647时，write-buffer-size 会变成负数的问题
Summary by CodeRabbit
Improvements
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.